-
Notifications
You must be signed in to change notification settings - Fork 1.1k
scaladoc: indicate optional parameters with = ...
#23676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
= ...
currently, there is no indication in the scaladoc when parameters may be optional. this leads to [long and overwhelming signatures][1] which is not at all user-friendly. users are forced to first intuit that this method *might* have some optional parameters, then manually find [the source code][2] to learn which parameters are optional. [1]: https://javadoc.io/static/com.lihaoyi/os-lib_3/0.11.5/os/proc.html#call-fffff910 [2]: https://github.com/com-lihaoyi/os-lib/blob/0.11.5/os/src/ProcessOps.scala#L192-L205 this PR suffixes `= ...` after the type signature of optional parameters. this makes it possible to tell that these parameters are optional and may be omitted. this applies to both method parameters and class parameters. the format is intentionally similar to the definition of such optional parameters in code: ```scala // new scaladoc display: def f(x: Int, s: String = ...): Nothing // code: def f(x: Int, s: String = "a"): Nothing ``` of course, the `...` term is different. i think this is a reasonable choice because (1) ellipsis commonly represents something present but omitted, and (2) it is not valid Scala, so there is no risk someone will think this is denoting a literal default value of `...`. a proper ellipsis character (rather than 3 periods) could also be considered, but i found that looked out of place amongst the monospace signature. about displaying the default value itself, this PR does not display the default value. this is because of anticipated difficulties around displaying an expression. this could be re-visited in future, but i think it should not hold up this PR. i believe that this PR alone is already a substantial improvement for the documentation of optional parameters.
bf0b9d1
to
a7d9437
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
The code looks good to me. I'm wondering if we should put this option under a flag (maybe one that is true by default).
I'll nitpick on part of the motivation: if one sets up "-source-links", looking up the source code is just one click, instead of a manual search.
Overall it looks like a nice improvement.
I don't think a flag is super necessary, but I'd be happy to add one if wanted. Do you have suggestions for a name? Re clicking the source links, many scaladocs are uploaded without this enabled. Also, I've had problems with the source link line numbers being incorrect - yet to file a bug though. |
I'm just slightly worried that in some cases, it will obscure the signature too much, but maybe you're right. Let's leave it as is for now and add a flag to disable it (or replace with full expressions) when someone complains. |
Thanks! What's the process to request a backport to LTS? |
We take a look at the merged prs once a week and if possible try to backport them. This time it might be a bit later, but it's all on one big list and will not get lost |
currently, there is no indication in the scaladoc when parameters may be optional. this leads to [long and overwhelming signatures][1] which is not at all user-friendly. users are forced to first intuit that this method *might* have some optional parameters, then manually find [the source code][2] to learn which parameters are optional. [1]: https://javadoc.io/static/com.lihaoyi/os-lib_3/0.11.5/os/proc.html#call-fffff910 [2]: https://github.com/com-lihaoyi/os-lib/blob/0.11.5/os/src/ProcessOps.scala#L192-L205 this PR suffixes `= ...` after the type signature of optional parameters. this makes it possible to tell that these parameters are optional and may be omitted. this applies to both method parameters and class parameters. the format is intentionally similar to the definition of such optional parameters in code: ```scala // new scaladoc display: def f(x: Int, s: String = ...): Nothing // code: def f(x: Int, s: String = "a"): Nothing ``` of course, the `...` term is different. i think this is a reasonable choice because (1) ellipsis commonly represents something present but omitted, and (2) it is not valid Scala, so there is no risk someone will think this is denoting a literal default value of `...`. a proper ellipsis character (rather than 3 periods) could also be considered, but i found that looked out of place amongst the monospace signature. about displaying the default value itself, this PR does not display the default value. this is because of anticipated difficulties around displaying an expression. this could be re-visited in future, but i think it should not hold up this PR. i believe that this PR alone is already a substantial improvement for the documentation of optional parameters. finally, here is a screenshot of the scaladoc from the new optionalParams.scala test case: <img width="663" height="646" alt="image" src="https://github.com/user-attachments/assets/870410ee-0535-484b-84c8-8b683464471e" /> this might be related to scala#13424
currently, there is no indication in the scaladoc when parameters may be optional. this leads to [long and overwhelming signatures][1] which is not at all user-friendly. users are forced to first intuit that this method *might* have some optional parameters, then manually find [the source code][2] to learn which parameters are optional. [1]: https://javadoc.io/static/com.lihaoyi/os-lib_3/0.11.5/os/proc.html#call-fffff910 [2]: https://github.com/com-lihaoyi/os-lib/blob/0.11.5/os/src/ProcessOps.scala#L192-L205 this PR suffixes `= ...` after the type signature of optional parameters. this makes it possible to tell that these parameters are optional and may be omitted. this applies to both method parameters and class parameters. the format is intentionally similar to the definition of such optional parameters in code: ```scala // new scaladoc display: def f(x: Int, s: String = ...): Nothing // code: def f(x: Int, s: String = "a"): Nothing ``` of course, the `...` term is different. i think this is a reasonable choice because (1) ellipsis commonly represents something present but omitted, and (2) it is not valid Scala, so there is no risk someone will think this is denoting a literal default value of `...`. a proper ellipsis character (rather than 3 periods) could also be considered, but i found that looked out of place amongst the monospace signature. about displaying the default value itself, this PR does not display the default value. this is because of anticipated difficulties around displaying an expression. this could be re-visited in future, but i think it should not hold up this PR. i believe that this PR alone is already a substantial improvement for the documentation of optional parameters. finally, here is a screenshot of the scaladoc from the new optionalParams.scala test case: <img width="663" height="646" alt="image" src="https://github.com/user-attachments/assets/870410ee-0535-484b-84c8-8b683464471e" /> this might be related to scala#13424 [Cherry-picked d240468][modified]
currently, there is no indication in the scaladoc when parameters may be optional. this leads to [long and overwhelming signatures][1] which is not at all user-friendly. users are forced to first intuit that this method *might* have some optional parameters, then manually find [the source code][2] to learn which parameters are optional. [1]: https://javadoc.io/static/com.lihaoyi/os-lib_3/0.11.5/os/proc.html#call-fffff910 [2]: https://github.com/com-lihaoyi/os-lib/blob/0.11.5/os/src/ProcessOps.scala#L192-L205 this PR suffixes `= ...` after the type signature of optional parameters. this makes it possible to tell that these parameters are optional and may be omitted. this applies to both method parameters and class parameters. the format is intentionally similar to the definition of such optional parameters in code: ```scala // new scaladoc display: def f(x: Int, s: String = ...): Nothing // code: def f(x: Int, s: String = "a"): Nothing ``` of course, the `...` term is different. i think this is a reasonable choice because (1) ellipsis commonly represents something present but omitted, and (2) it is not valid Scala, so there is no risk someone will think this is denoting a literal default value of `...`. a proper ellipsis character (rather than 3 periods) could also be considered, but i found that looked out of place amongst the monospace signature. about displaying the default value itself, this PR does not display the default value. this is because of anticipated difficulties around displaying an expression. this could be re-visited in future, but i think it should not hold up this PR. i believe that this PR alone is already a substantial improvement for the documentation of optional parameters. finally, here is a screenshot of the scaladoc from the new optionalParams.scala test case: <img width="663" height="646" alt="image" src="https://github.com/user-attachments/assets/870410ee-0535-484b-84c8-8b683464471e" /> this might be related to #13424 [Cherry-picked d240468]
currently, there is no indication in the scaladoc when parameters may be optional. this leads to long and overwhelming signatures which is not at all user-friendly. users are forced to first intuit that this method might have some optional parameters, then manually find the source code to learn which parameters are optional.
this PR suffixes
= ...
after the type signature of optional parameters. this makes it possible to tell that these parameters are optional and may be omitted.this applies to both method parameters and class parameters. the format is intentionally similar to the definition of such optional parameters in code:
of course, the
...
term is different. i think this is a reasonable choice because (1) ellipsis commonly represents something present but omitted, and (2) it is not valid Scala, so there is no risk someone will think this is denoting a literal default value of...
. a proper ellipsis character (rather than 3 periods) could also be considered, but i found that looked out of place amongst the monospace signature.about displaying the default value itself, this PR does not display the default value. this is because of anticipated difficulties around displaying an expression. this could be re-visited in future, but i think it should not hold up this PR. i believe that this PR alone is already a substantial improvement for the documentation of optional parameters.
finally, here is a screenshot of the scaladoc from the new optionalParams.scala test case:

this might be related to #13424